Skip to content

Conversation

@iskiselev
Copy link
Contributor

@iskiselev iskiselev commented Oct 7, 2025

Why

Currently OTEL registers in GAC assemblies, that was never intendend to be put in GAC on latest framework - and not registering netstandard.dll on 4.6.2. It results in issues discussed at #4186 (comment)

What

With this change, we pack different set of assemblies for supported .NET Frameworks:

  • 4.6.2
  • 4.7
  • 4.7.1
  • 4.7.2 and up

Only assemblies for current framework will be registered in GAC at install time or loaded from installation dir.
To minimize size set of assemblies deduplicated - with assemblies that used in all .NET Frameworks still stay at netfx folders. Assemblies used only in some versions stays in oldest TFM folder. For other TFM using same assembly .link file will be created that points to TFM with assembly.
To simplify buget packages tracking CentralPackageTransitivePinningEnabled for newly created OpenTelemetry.AutoInstrumentation.Assemblies.NetFramework.csproj was enabled - with it transient auto-generated packages were removed from OpenTelemetry.AutoInstrumentation.csproj.
.NET Framework version resolved through registry check in installation, profiler and managed code. It associated with some risk (proper rights). If version will not be resolved, it is fallback to 4.6.2, which should have same behaviour as before this change. It may be better nowdays to fallback to 4.7.2 - as most people probably have it.
Deployment for .Net Frameworks higher than 4.7.2 are same as for 4.7.2, so we do not build them - but test added to validate it.

Tests

  • validated OTEL works when assemblies are loaded from install folder on .NET 4.7.2.
  • validated that .link files are loaded correctly
  • validated that GAC installation works properly
  • validated that nuget installation was not affected

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

Open questions

  • Should we fallback to 4.7.2 instead of 4.6.2 if .Net Framework version not detected?
  • Decide if we need any replacement for deleted TransientDependenciesTests.DefinedTransientDeps_Are_MatchingGeneratedDeps. Do we need it once we start utilize CentralPackageTransitivePinningEnabled?
  • We don't have reference/version pinning in global/src level of Directory.Packages.props for some transitive packages.
    Should we have it? Currently they are added as other transitive dependencies in OpenTelemetry.AutoInstrumentation.Assemblies.NetFramework/Directory.Packages.props. We can prevent it by restoring !dep.Name.StartsWith("OpenTelemetry") && dep.Name != "OpenTracing" in Generator, but pinning them through higher level Directory.Packages.props may be better idea. Packages to discuss:
    • OpenTelemetry.Api.ProviderBuilderExtensions
    • OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule
    • OpenTracing
  • Should we integrate OpenTelemetry.AutoInstrumentation.Assemblies.NetFramework/Directory.Packages.props with dependabot? Priory only dependencies in top-level Directory.Packages.props were processed.
  • More automation tests? Suggest what to cover.

@iskiselev iskiselev requested a review from a team as a code owner October 7, 2025 03:28
@iskiselev iskiselev marked this pull request as draft October 7, 2025 03:28
…ation.Assemblies.NetFramework

It partially resolve open-telemetry#4520 by making both NuGet and zip version of OpenTelemetry.AutoInstrumentation reference same versions of dependencies,
but not change assembly redirections or versions included in zip.

Switched DependencyListGenerator to use dotnet list package internally. Validate defined PackageVersion through MsBuild evaluation.
@iskiselev iskiselev changed the title WIP Use framework-specific dependencies for .Net Framework versions Use framework-specific dependencies for .Net Framework versions Oct 16, 2025
@iskiselev iskiselev marked this pull request as ready for review October 16, 2025 23:24
private static readonly IEnumerable<TargetFramework> TargetFrameworks = new[]
{
TargetFramework.NET8_0,
TargetFramework.NET462,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFramework.NET462 should this remain here? Becomes quite confusing between TargetFrameworks vs TargetFrameworksNetFx

Copy link
Contributor Author

@iskiselev iskiselev Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should remain here.
Probably we should find better name for TargetFrameworksNetFx.
TargetFrameworks - TFM used for AutoInstrumentation compilation
TargetFrameworksNetFx - TFM list used to pack AutoInstrumentation for .Net Framework (packing for .NET has different, unchanged mechanic).

Copy link
Member

@Kielek Kielek Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFrameworksForNetFxRedirections?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TargetFrameworksForNetFxPacking?

private string MapToFolderOutput(TargetFramework targetFramework)
{
return targetFramework.ToString().StartsWith("net4") ? "netfx" : "net";
return targetFramework.ToString().StartsWith("net4") ? $"netfx" : "net";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return targetFramework.ToString().StartsWith("net4") ? $"netfx" : "net";
return targetFramework.ToString().StartsWith("net4") ? "netfx" : "net";

public static readonly TargetFramework NET462 = new() { Value = "net462" };
public static readonly TargetFramework NET47 = new() { Value = "net47" };
public static readonly TargetFramework NET471 = new() { Value = "net471" };
public static readonly TargetFramework NET472 = new() { Value = "net472" };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need net48 and net481 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we don't need net48 and net481, as all of our dependencies does not have difference between 4.7.2 and higher. I've added new NetFrameworkDistroTests.ReferencedPackagesNoUnsupportedTfm test that validates it. If any dependencies will do customizations for 4.8, 4.8.1 or potential newer .Net Framework - it will fail. There is comment in tests what should be done in that case.

else
{
Logger::Warn(
"DetectFrameworkVersionTableForRedirectsMap: Old .Net Framework detected, use 462 as fallback");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"DetectFrameworkVersionTableForRedirectsMap: Old .Net Framework detected, use 462 as fallback");
"DetectFrameworkVersionTableForRedirectsMap: Old .NET Framework detected, use 462 as fallback");

@nrcventura
Copy link
Member

Should we fallback to 4.7.2 instead of 4.6.2 if .Net Framework version not detected?

As was described in the SIG meeting, I think falling back to 4.6.2 makes sense to maintain the existing the behavior. However, I'm wondering if we can improve upon it for those using the install script? Maybe the install script is run with enough permissions to configure an environment variable to modify the fallback behavior if a newer version of .net framework is detected at install time. If so, something like that could be implemented separately.

int frameworkVersion = 462;

HKEY hKey = nullptr;
LONG result = RegOpenKeyExW(HKEY_LOCAL_MACHINE, L"SOFTWARE\\Microsoft\\NET Framework Setup\\NDP\\v4\\Full\\", 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The registry check logic is in multiple locations. Is it possible to just have it checked in one place and made available in both locations?

@nrcventura
Copy link
Member

More automation tests? Suggest what to cover.

One idea for a set of integration tests is having a test app that runs in windows containers with only a specific .net framework version available to make it easier to validate behavior for each specific .net framework version. I don't know what type of test app we need to have to ensure that the correct dependencies are in place. I also don't know if these tests will have enough value given how long it can take to download and run these containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants